fix(platform): reject path traversal and harden fs cache and retry boundaries - #3315

Pull request overview

This PR hardens the filesystem adapters (GitHub + Veryfront API) against path traversal, cache-collision, unsafe error introspection, and retry-boundary misuse, and adds regression tests to lock in the security/robustness fixes.

Changes:

• Reject .. traversal segments (while normalizing away .) and fix projectDir prefix stripping to only match complete path-segment boundaries.
• Scope GitHub cache keys by owner:repo:ref to prevent cross-repo cache collisions, plus add isolation tests.
• Make FS cache sizing resilient to non-serializable values, and harden retry transient-error detection to avoid invoking attacker-controlled getters.

Verification

• I did not run commands in this review environment. Safest next step: run the adapter-focused suite mentioned in the PR description (plus deno lint / deno fmt --check on changed files) after addressing the review comments.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

      ttl?: number;
    };
    retry?: {
      /** Retries after the initial request, from 0 through 9. */
      maxRetries?: number;
      initialDelay?: number;
      maxDelay?: number;
    };

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

      /** Retries after the initial request, from 0 through 9. */

Actionable comments posted: 3

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/platform/adapters/fs/veryfront/adapter-helpers.ts`:
- Around line 20-28: Update buildRetryConfig to catch validation failures from
normalizeFilesystemRetryConfig and wrap them in a VeryfrontError with slug
"config-validation-failed"; preserve and rethrow existing matching
VeryfrontError instances using the specified instanceof and slug check. Ensure
createFSAdapterFromConfig propagates this registered error and
enhanceAdapterWithFS does not silently fall back to the local filesystem when
retry configuration is invalid.

In `@src/platform/adapters/fs/veryfront/path-normalizer.ts`:
- Around line 17-20: Update the constructor and normalization flow around
projectDirPrefix and normalize so both the configured project directory and
input path are canonicalized before project-prefix boundary comparison,
including removal of "." segments while preserving "/" root handling. Keep the
public API unchanged, and add a focused regression test covering canonical and
dot-segment forms of the same configured project directory.

In `@src/platform/adapters/fs/veryfront/types.ts`:
- Around line 126-131: Update the retry documentation in the retry configuration
type to reference MAX_VERYFRONT_FILESYSTEM_RETRIES instead of hardcoding 9,
keeping the documented range accurate if the constant changes. Remove retryDelay
from the filesystem adapter README and document the current FSAdapterConfig
fields initialDelay and maxDelay instead.

---

Nitpick comments:
In `@src/platform/adapters/fs/github/read-operations.test.ts`:
- Line 5: Update the FileCache import in the test to use the `#veryfront/cache`
alias, which resolves through src/cache/index.ts and exports FileCache, instead
of the relative cache path.

In `@src/platform/adapters/fs/veryfront/adapter-helpers.test.ts`:
- Around line 46-62: Update the retry-delay tests around buildRetryConfig: pass
each retry input as the failure message to assertThrows so the failing case is
identifiable, and move the zero-delay acceptance assertion into a separate it()
test with a title describing accepted zero delays.

In `@src/platform/adapters/fs/veryfront/retry.test.ts`:
- Around line 175-198: Update the test title in the hostile throwable test to
describe that the value is rethrown unchanged without triggering introspection
traps. Import assertStrictEquals from `#veryfront/testing/assert.ts` and replace
the boolean comparison of caught and hostile with a direct strict identity
assertion, preserving the existing call-count check.

In `@src/platform/adapters/fs/veryfront/retry.ts`:
- Around line 84-116: Remove the redundant message.includes("network error")
condition and its associated comment from the isNativeTypeError branch. Keep the
shared network error check and explanatory comment in the broader retry
classification condition unchanged.
- Around line 136-138: Update the warning payload in the retry flow around
getSafeErrorMessage so an empty safe error message falls back to the throwable’s
typeof value. Preserve the existing safe message when it is non-empty and ensure
the error field always provides this diagnostic fallback for non-native
throwables.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.

  } catch (error) {
    if (error instanceof VeryfrontError && error.slug === "config-validation-failed") {
      throw error;
    }

      } catch (error) {
        if (error instanceof VeryfrontError && error.slug === "config-validation-failed") {
          throw error;
        }

Review: 54/100 — request changes

Merge base c21affb69c5b, head ed1a04d1d. All diffs three-dot from the explicit merge base.

The vulnerability is real and correctly diagnosed. The fix is incomplete: the guard blocks only the literal .. form, and I empirically confirmed four inputs that produce the exact escape the PR says it closes. Main is fully open, so this is a net improvement — but the PR body and its tests assert a completeness that does not exist, which is the "incomplete guard creates false confidence" case.

P0 — traversal guard bypassable via percent-encoded dot segments and backslashes

src/platform/adapters/fs/github/path-utils.ts:23-31

Confidence: high — empirically executed against the real endpoint-construction logic.

The sink at github-api-client.ts:61-63 interpolates the path raw:

const normalizedPath = path.replace(/^\/+/, "");
const endpoint = `/repos/${this.config.owner}/${this.config.repo}/contents/${normalizedPath}?ref=${contentRef}`;

There is no encodeURIComponent anywhere in that file (verified by grep). github-api-client.ts:131 then does new URL(...) and fetch, with the auth header attached at :176.

The guard rejects a segment only when it is exactly ... But the WHATWG URL Standard defines a double-dot path segment as .., .%2e, %2e., or %2e%2e (ASCII case-insensitive), and treats \ as a path separator for special schemes. I reproduced the real sink and got:

blocked       | escaped=YES | "../../../../user/repos"                  -> https://api.github.com/user/repos?ref=main
PASSES-GUARD  | escaped=YES | "%2e%2e/%2e%2e/%2e%2e/%2e%2e/user/repos"  -> https://api.github.com/user/repos?ref=main
PASSES-GUARD  | escaped=YES | "%2E%2E/%2E%2E/%2E%2E/%2E%2E/user/repos"  -> https://api.github.com/user/repos?ref=main
PASSES-GUARD  | escaped=YES | ".%2e/.%2e/.%2e/.%2e/user/repos"          -> https://api.github.com/user/repos?ref=main
PASSES-GUARD  | escaped=YES | "..\..\..\..\user/repos"                  -> https://api.github.com/user/repos?ref=main
blocked       | escaped=no  | "docs/readme.md"                          -> .../contents/docs/readme.md?ref=main

Concrete failure: caller invokes adapter.readTextFile("%2e%2e/%2e%2e/%2e%2e/%2e%2e/user/repos"). normalizeGitHubPath returns it unchanged (no segment equals ..). read-operations.ts:40 passes it to readContentsFile → the URL resolves to https://api.github.com/user/repos?ref=main carrying Authorization: Bearer <GITHUB_TOKEN>. The full repo list visible to that token is returned to the caller as "file contents". Substituting /user, /orgs/{org}/members, etc. reaches any GET the token can perform. Reachability is exactly as the PR states for the literal form: readTextFile → readContentsFile whenever the tree index lacks the path.

The fix belongs at the sink, and it needs BOTH layers. I tested the encoding fix and it does not subsume the .. rejection:

# segment-wise encodeURIComponent at github-api-client.ts:62
ESCAPED !! | "../../../../user/repos"                 -> https://api.github.com/user/repos?ref=main
CONTAINED  | "%2e%2e/%2e%2e/%2e%2e/%2e%2e/user/repos" -> .../contents/%252e%252e/%252e%252e/...
CONTAINED  | "%2E%2E/..."                             -> .../contents/%252E%252E/...
CONTAINED  | ".%2e/.%2e/..."                          -> .../contents/.%252e/.%252e/...
CONTAINED  | "..\..\..\..\user/repos"                 -> .../contents/..%5C..%5C..%5C..%5Cuser/repos
ok         | "docs/read me.md"                        -> .../contents/docs/read%20me.md
ok         | "src/a#b.ts"                             -> .../contents/src/a%23b.ts

encodeURIComponent("..") returns ".." unchanged — dots are unreserved characters. So encoding closes the encoded and backslash variants but leaves the literal ../ escape wide open, and the existing .. rejection closes the literal form but nothing else. Each layer covers exactly what the other misses.

Recommended: apply path.split("/").map(encodeURIComponent).join("/") at github-api-client.ts:62 and keep the .. segment rejection — it is load-bearing, not defence in depth. Bonus: encoding also fixes paths containing spaces, #, and ?, which are broken on main today.

P1 — the GitHub normalizer omits the control-char/backslash/length checks the Veryfront one has

src/platform/adapters/fs/github/path-utils.ts

The PR body states this asymmetry as intentional. But the GitHub adapter is the one whose sink is unencoded, so it needs them more. PathNormalizer.assertSafePath (path-normalizer.ts:79-98) rejects \; normalizeGitHubPath does not — which is exactly why the ..\..\ bypass works against GitHub and not against Veryfront.

Scope note worth recording: the Veryfront adapter was already safe at the URL layer — veryfront-api-client/operations.ts:326,363,442,478 all use encodeURIComponent(pathOrId). So the traversal half of this PR delivers real value only for GitHub, and there it is incomplete.

P1 — hostile projectDir fails open to the local filesystem

src/platform/adapters/fs/integration.ts:67

The new guard rethrows only VeryfrontError with slug config-validation-failed. But PathNormalizer's constructor throws a plain TypeError (path-normalizer.ts:80-97), reached from veryfront/adapter.ts:270 during createFSAdapter.

Concrete failure: config sets fs.veryfront.projectDir = "/project/../etc". The constructor throws TypeError. integration.ts:67 does not match → falls through to :70-76 → logs "Falling back to local filesystem" → returns the unenhanced Deno adapter. The app now serves from the real local filesystem with no project scoping at all, silently, behind a warn line. Validation intended to harden the boundary instead removes it.

The config-validation-failed rethrow only covers the retry path (adapter-helpers.ts:33), which is what integration.test.ts:103-117 actually exercises. There is no test for the PathNormalizer-throw path.

P2 — previously-booting configs now fail to boot

src/platform/adapters/fs/veryfront/adapter-helpers.ts:22-30

normalizeFilesystemRetryConfig throws; it does not clamp (config-resource-limits.ts:70-84, 100-127). MAX_VERYFRONT_FILESYSTEM_RETRIES = 9.

Concrete failure: an existing app with fs.veryfront.retry.maxRetries: 10 boots fine on main (bare spread). After this PR: RangeError → CONFIG_VALIDATION_FAILED → rethrown by adapter-helpers.ts:33 → rethrown by integration.ts:67, bypassing the local-fs fallback → enhanceAdapterWithFS rejects → the app fails to start. Same for initialDelay: 20000 against the default maxDelay: 10000.

Fail-fast is defensible, but this is a breaking runtime change framed as "hardening" with no migration note. Please scan deployed fs.veryfront.retry values for maxRetries > 9 or initialDelay > maxDelay before merging, and add a changelog entry.

P2 — the cache size-estimator guard is a no-op on the production backend

src/platform/adapters/fs/cache/size-estimator.ts:5-15

Stated goal: stop cyclic/BigInt/throwing-toJSON values propagating out of FileCache.set(). But file-cache.ts:201-217 computes estimateSize (now returning MAX_SAFE_INTEGER instead of throwing) and then calls JSON.stringify(entry) at :208, which rethrows the identical exception whenever a distributed backend is configured — production uses Upstash Redis. The guard only helps the in-memory path. Note setAsync at :241 does wrap the stringify in try/catch; the sync set does not.

Mitigating: these caches hold string, Uint8Array, DirectoryEntry[], FileInfo, string | null — all plain parsed-JSON data, so a cyclic value is not reachable today. Defence-in-depth against a hypothetical, implemented incompletely.

P3s

• src/platform/adapters/fs/veryfront/types.ts:127-129 — retryDelay → initialDelay/maxDelay. I verified retryDelay was genuinely dead at the merge base (only types.ts:128 and README.md:240, no consumer), so the PR's claim is true and the change is correct. But FSAdapterConfig is exported, so a downstream app setting veryfront.retry.retryDelay gets a compile error on upgrade. Needs a semver/changelog note.
• src/platform/adapters/fs/veryfront/retry.ts:37-40 — getSafeErrorMessage returns "" for non-native errors. An error built as Object.create(Error.prototype) satisfied main's instanceof Error and yielded error.message; it now yields "", so ECONNRESET-style matching silently stops working. Narrow — no such producer found in-repo.

What is correct — verified, worth keeping

• Segment-boundary projectDir stripping (path-utils.ts:5-12, path-normalizer.ts:44-52). Main's bare startsWith genuinely mis-stripped /project/root against /project/root-other. Fixed and tested in both adapters.
• Repo-scoped cache keys (cache-scope.ts) — correct, and the wiring is complete: all five bare-ref key sites updated (directory-operations.ts:21, read-operations.ts:41,61,100, stat-operations.ts:132,189), and buildGitHubTreeCacheKey already took repoId so was correctly left alone.
• All six GitHub entry points normalize. The adapter is read-only (adapter.ts:97-127); no write/delete surface exists to miss, and no unguarded alternate path into getContents.
• Symlink-skip preservation claim is true — the stat-operations.ts diff contains only cache-key changes.
• The .-segment decision is well-reasoned and correctly documented. Retry status window 500–599 integer-bounded, and descriptor-based reads, are real improvements.

Test adequacy

Negative coverage exists and is decent for what it targets: path-utils.test.ts:53-75, path-normalizer.test.ts:104-153 (traversal, backslash, control chars, 4097-char bound, segment boundary), cross-repo cache isolation, integration.test.ts:103-117, size-estimator.test.ts:57-81.

The gaps map exactly onto the findings:

1. No test for encoded traversal (%2e%2e, %2E%2E, .%2e) or backslash traversal on the GitHub side — the actual hole. The tests assert the literal form is blocked, which is precisely what produces the false confidence.
2. No test at the sink. A single test doing new URL(baseUrl + endpoint) and asserting the pathname still starts with /repos/OWNER/REPO/ would have caught all four bypasses at once. That is the test to add.
3. No test for the PathNormalizer-throw → local-fs fallback.
4. No test that FileCache.set survives a cyclic value with a backend configured.

Production risk & rollback

Rollback clean — pure code, no migrations, no persisted-format change. The cache-key change means post-revert lookups miss once and refill; harmless.

• Deploy-blocking: the retry-config validation can turn a running app into a boot failure. Check deployed configs first.
• Security posture: net improvement over main, but the title and body would justify closing this as "traversal fixed" when it is not. If you merge before fixing, please amend the description so the residual bypass is not lost.
• Silent failure: the local-fs fallback degrades isolation with only a warn line as signal.

Minimum to reach merge-ready

1. Encode path segments at github-api-client.ts:62 and keep the .. rejection — verified above that neither alone is sufficient.
2. Add the sink-level assertion test plus the encoded and backslash cases.
3. Make integration.ts:67 rethrow path-validation failures too, or have PathNormalizer throw CONFIG_VALIDATION_FAILED.
4. Either wrap the JSON.stringify at file-cache.ts:208 or drop the cache-guard claim.
5. Changelog notes for the retry-config and retryDelay breaking changes.

Critical Review — Score: 62/100

Verdict

The retry-boundary and cache-scoping work is careful, well-argued and well-tested, and the segment-boundary projectDir fix is a real bug fix. But the headline security claim does not hold: the GitHub traversal check rejects only path segments that are literally .., while the sink interpolates the path raw into a URL string, and the same WHATWG parsing behaviour the PR description relies on ("WHATWG URL resolution collapses dot segments") also collapses percent-encoded dots, treats backslashes as separators, and strips embedded tab/newline before dot-segment removal. The vector the PR was written to close is still open through three trivial encodings, and none of them are tested. Rubric band: 50–74, needs changes before merge.

Findings

1. [blocker] normalizeGitHubPath is bypassable with %2e%2e, \, or embedded tab/newline — the exact vector C1 claims to close.
   src/platform/adapters/fs/github/path-utils.ts rejects only exact-match segments:

   for (const segment of collapsed.split("/")) {
     if (segment === ".") continue;
     if (segment === "..") { throw new TypeError(...); }

   The sink does no encoding — src/platform/adapters/fs/github/github-api-client.ts:63-65:

   const normalizedPath = path.replace(/^\/+/, "");
   const endpoint = `/repos/${owner}/${repo}/contents/${normalizedPath}?ref=${contentRef}`;

   followed by fetch(${this.baseUrl}${endpoint}) (line 136/140), i.e. a WHATWG URL parse of a string the caller controls. Per the URL Standard, a double-dot path segment is .. or an ASCII case-insensitive match for .%2e, %2e., or %2e%2e; a single-dot path segment is . or %2e. Separately, the basic URL parser removes all ASCII tab/LF/CR from the input before parsing, and for special schemes (https:) \ is a path separator. So all of these pass the new check and still collapse into traversal against a token-authenticated api.github.com request:

   • %2e%2e/%2e%2e/%2e%2e/user/repos (also %2E%2E, .%2e, %2e.)
   • ..\..\..\user\repos (one split("/") segment, never equal to ..)
   • .<TAB>./.<LF>./user/repos (segments are .\t., not .., until the parser strips the control chars)

   Reachability is unchanged from the PR's own analysis: readTextFile → getFileEntry miss (read-operations.ts:50) → readContentsFile → client.getContents(normalizedPath). The fix needs to reject on a decoded/parsed view (e.g. reject any % / \ / control char in a path segment, or encodeURIComponent each segment at the client, as the Veryfront client already does), and path-utils.test.ts needs cases for all three encodings — today it only tests literal ../.

2. [major] The strict validator was put on the safe adapter and the lax one on the dangerous adapter.
   PathNormalizer.assertSafePath (fs/veryfront/path-normalizer.ts) rejects control characters, \, and >4096-char paths — but Veryfront paths are already made inert by encodeURIComponent(pathOrId) at every call site in src/platform/adapters/veryfront-api-client/operations.ts (lines 326, 363, 442, 480, 540, 578). normalizeGitHubPath has none of those checks, and it is the one whose output is interpolated unencoded (finding 1). The hardening is inverted relative to risk; at minimum the control-char/backslash/length checks belong in path-utils.ts too.

3. [major] C2's guarded serialization does not cover the mode the file header calls production.
   size-estimator.ts now returns Number.MAX_SAFE_INTEGER instead of throwing, but FileCache.set() (fs/cache/file-cache.ts:206-215) does, in the distributed branch:

   const backend = this.getBackend();
   if (backend) {
     const serialized = JSON.stringify(entry);   // unguarded

   A cyclic value or BigInt still throws straight out of FileCache.set() whenever a Redis/API backend is active — precisely the failure the PR says it fixed. The "uncacheable, rejected by admission limits" claim also only holds for the memory path (setToFallback, line 255, size > this.options.maxMemory); in distributed mode the MAX_SAFE_INTEGER size is only used as a span attribute (line 249) and the entry is written anyway. Either guard line 208 the same way, or have set/setAsync skip admission when estimateSize returns the sentinel. No test covers the backend branch.

4. [minor] Retry-config hardening is one-sided: the GitHub adapter's boundary is still unvalidated.
   buildRetryConfig now routes through normalizeFilesystemRetryConfig, but createGitHubConfig (fs/github/types.ts:103-107) still does a bare

   maxRetries: config.retry?.maxRetries ?? DEFAULT_MAX_RETRIES,

   and that value is passed as maxAttempts in github-api-client.ts:157 and :115. retryWithBackoff only rejects non-integers/< 1 (errors/error-handlers.ts:166-170), so {maxRetries: 1_000_000} is accepted and hammers the GitHub API — finite, but exactly the class of unbounded-budget bug the PR fixed on the sibling adapter. config-resource-limits.ts already ships MAX_GITHUB_FILESYSTEM_ATTEMPTS and the "legacy-total-attempts" semantics for this boundary; they are unused here.

5. [minor] Cache-key scoping is incomplete and inconsistent with its own encoder.
   cache-scope.ts correctly encodeURIComponents owner:repo:ref, but the highest-value poisoning target — the whole file index — still uses the unencoded key at fs/github/stat-operations.ts:55: buildGitHubTreeCacheKey(this.client.repoId, this.config.ref) where repoId is `${owner}/${repo}`. {repo: "b:main", ref: "x"} and {repo: "b", ref: "main:x"} both produce github:tree:a/b:main:x. Same for github:blob:${sha} / github:blob:bytes:${sha} (read-operations.ts:195,209), left unscoped — defensible because git blob SHAs are content-addressed, but the PR text claims to enumerate the key-builder call sites and doesn't mention either. The path component of every key also stays unencoded, so a path containing :exact: can alias the bounded-read key built at read-operations.ts:99-101; the repo already has src/cache/keys/segment-codec.ts for exactly this.

6. [minor] projectDir is stripped twice on the GitHub read/readdir paths.
   directory-operations.ts:20 normalizes, then passes the already-normalized path into statOps.isDirectory / getFilesInDirectory / getSubdirectories, each of which calls normalizeGitHubPath(path, this.projectDir) again (stat-operations.ts:224,228,243,259); read-operations.ts:50 does the same via getFileEntry. With projectDir: "app", /app/app/page.tsx normalizes to app/page.tsx and then to page.tsx — a different file — while the cache key (directory-operations.ts:21-24) was built from the once-normalized form, so key and lookup disagree. Pre-existing, but this PR is the one that rewrote projectDir stripping semantics and claims to have audited these call sites.

7. [minor] integration.ts fail-fast covers one slug only.
   The new guard (fs/integration.ts:67-69) rethrows only config-validation-failed. A missing/invalid GitHub token throws CONFIG_INVALID (fs/github/adapter.ts:37,52), which still falls into the original catch and silently swaps the site onto denoAdapter — the "changing filesystems" hazard named by the new test (integration.test.ts, "should preserve invalid retry configuration instead of changing filesystems") remains open for every other configuration error.

8. [nit] The new cache-isolation tests can pass vacuously.
   directory-operations.test.ts and read-operations.test.ts build new FileCache() and rely on the sync fallback. cacheBackend is module-global (file-cache.ts:49) and get() returns a miss unconditionally when a backend is set (file-cache.ts:134-137), so if any earlier test in the process calls initializeFileCacheBackend(), both assertions pass whether or not the scoping fix exists. Since the stated real-world exposure is the shared distributed backend, that path is the one left untested.

9. [nit] new PathNormalizer("") makes every absolute path "in project".
   path-normalizer.ts guards with projectDir !== undefined, so an empty string yields projectDirPrefix === "" and then normalizedPath.startsWith(${projectDir}/) is startsWith("/") — true for any absolute path. The slice(0) is a no-op so behaviour is correct, but it logs "Converted absolute to relative path" for every read. Cheap fix: treat "" like undefined.

What's good

• The retry-classification rewrite is genuinely correct: getOwnPropertyDescriptor-based reads, the integer 500–599 window, and the native-error/proxy checks close real getter-invocation and status: Infinity holes, and the three new tests in retry.test.ts pin exactly those behaviours. The single call site (file-list-access.ts:95, a file list) is idempotent and maxAttempts: 2 — no unbounded or non-idempotent retry anywhere in this diff.
• Routing buildRetryConfig through the existing normalizeFilesystemRetryConfig rather than inventing new validation, plus dropping the vestigial retryDelay field from types.ts/README so overrides are actually expressible, is the right layering and removes a baseline typecheck exclusion.
• The startsWith(projectDir) → segment-boundary fix is a real correctness bug fix in both adapters, and the /project/root vs /project/root-other tests pin it precisely.

🤖 Critical review by Claude Code